switch pipelines and remote resolver controllers on prod m01 to state… #5398
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jkhelil The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Strictly speaking that configuration slightly differs from the one in staging where tekton-pipelines-remote-resolvers is missing from the list of statefulSets. It is still working though as I see the statefulSet exist on the cluster. Ideally we'll have matching configurations in staging and prod. I do not mind which changes as far as they match. (I know that because I'm working on the same change, but you beat me to it).
| spec: | ||
| containers: | ||
| - args: | ||
| - --threads-per-controller=32 |
There was a problem hiding this comment.
Is that really needed? I'm looking at the existing deployment on m01 and I see the following parameters passed to the container -kube-api-burst=50 -kube-api-qps=50 -threads-per-controller=32 --threads-per-controller=32 . Isn't enough we have threads-per-controller in the performance field?
There was a problem hiding this comment.
As you can see, I moved the sections for tekton-pipelines-controller and tekton-pipelines-remote-resolvers under spec/pipeline/statefulSets. This ensures that the topologySpreadConstraints, as well as resources/limits and requests, are applied to both controllers.
The threads-per-controller=32 setting should now be superseded by the performance flags defined under spec/pipeline/performance.
On the staging environment, we already have the same configuration for tekton-pipelines-controller. However, at the time I made this change, there was nothing related to remote-resolvers under spec/pipeline/deployments, as this section had already been moved to spec/pipeline/statefulSets
the change is conform to the existant, and yes it is slightly different from stage (for remote-resolvers, because i guess on stage we didnt configure remote-resolvers limits and requests)
if you are trying to align stage with prod, lets do your PR before this then, we can hold this for a bit
There was a problem hiding this comment.
The threads-per-controller=32 setting should now be superseded by the performance flags defined under spec/pipeline/performance.
That was my point, given that threads-per-controller=32 is already set through spec/pipeline/performance and it is indeed passed to the container, we do not need it here.
On the staging environment, we already have the same configuration for tekton-pipelines-controller. However, at the time I made this change, there was nothing related to remote-resolvers under spec/pipeline/deployments, as this section had already been moved to spec/pipeline/statefulSets
I see here https://github.com/redhat-appstudio/infra-deployments/pull/5036/files#diff-e961bdc3a405dfda1b39ed17ee82576bb68e2a35e9288e7de208e695e4ee5944L2226-L2228 is removed from deployments and not added to statefulSets. This change includes it. If we indeed need it, then we should update the staging overlay accordingly. I do not want to mix prod and staging changes, so I can open a separate PR to update staging. Once we deploy and validate it, we can proceed with this change.
if you are trying to align stage with prod, lets do your PR before this then, we can hold this for a bit
There was a problem hiding this comment.
@enarha sound good to me, I wil wait for your PR on stage, once merged, we proceed with this one
There was a problem hiding this comment.
OK, I'll open one in a min. But what about the --threads-per-controller=32 (my original comment). Do you agree it should be removed?
There was a problem hiding this comment.
Yes I will removed it
|
/hold |
jkhelil
force-pushed
the
update_prod_m01_sts
branch
2 times, most recently
from
db8a8f0 to
9b905d4
Compare
jkhelil
force-pushed
the
update_prod_m01_sts
branch
from
9b905d4 to
d131e60
Compare
|
/unhold |
|
/hold |
…fulSets
linked to redhat-appstudio/o11y#462